Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #897: Username is shown as null on login #915

Merged
merged 1 commit into from
Nov 11, 2018

Conversation

etlhsu
Copy link
Contributor

@etlhsu etlhsu commented Oct 26, 2018

Fixes #897

The welcome toast shows the "Welcome" with the username, instead of "Welcome null". I solved the issue by addressing an error in the Retrofit implementation. The change helps Retrofit recognize the field properly.
screenshot_20181023-215845

@etlhsu
Copy link
Contributor Author

etlhsu commented Oct 26, 2018

@miPlodder, I believe my pull request is ready to merge. Can you help merge it or give me any improvements needed?

@@ -11,6 +12,7 @@ data class User (

var userId: Long = 0,
var isAuthenticated: Boolean = false,
@SerializedName(value = "username")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethan627hsu It's also a correct solution. But Instead, rename userName to username.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My solution has an advantage over the brute force, I don't have to refactor getters and setters across different files. My solution causes the least amount of overlap with different files.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that, but it's just a typing mistake.

Copy link
Contributor Author

@etlhsu etlhsu Oct 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your suggesting that I rename the field and refactor it across all of its references?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, moreover it not used much.

@etlhsu
Copy link
Contributor Author

etlhsu commented Oct 27, 2018

@miPlodder, I successfully implemented your strategy. Can you review my pull request for any other need modifications before merging?

@miPlodder
Copy link
Collaborator

@ethan627hsu Good job. Submit your task, I will approve it.

@miPlodder miPlodder merged commit e203fd6 into openMF:development Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants